Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for stat(2) path traversal using fstat #138

Closed
wants to merge 1 commit into from

Conversation

jaromil
Copy link

@jaromil jaromil commented May 18, 2020

Hi there!

Here is my fix to retrieve correct file information on POSX systems when along the path traversal a directory has no executable flag.

From stat(2) manpage:

"No permissions are required on the file itself, but—in the case of stat(), fstatat(), and lstat() execute (search) permission is required on all of the directories in pathname that lead to the file."

At the moment this is an incomplete fix: it breaks usage of lstat(2) on POSIX systems, due to change of the function signature passed to _file_info_().

Kindly provide feedback on how to proceed. IMHO the usage of a function pointer passed as argument of file_info is a design burden: a simple flag can be used.

ciao

p.s. for the impatient, a quick non-breaking fix is applied in the lfs version shipped in my software.

return(3);
}
#endif
if (st(fd, &info)) {
lua_pushnil(L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also close file inside if statement

@hishamhm
Copy link
Member

@jaromil Feel free to simplify the internals if it aids with the bugfix and preserves portability!

@jaromil
Copy link
Author

jaromil commented May 19, 2020

Thanks for your feedbacks, knowing your interest now I'll rework the PR into something more presentable and passing tests.

@hishamhm hishamhm force-pushed the master branch 2 times, most recently from 9cd10fe to f77f248 Compare June 24, 2022 15:10
@FractalU
Copy link
Contributor

FractalU commented Nov 7, 2024

@jaromil Your fix doesn't solve anything. In fact, it makes the code a lot worse in maintainability, readability and in functionality.

Let me explain. From reading your proposal and reviewing your changes I see that you try to convince us to use fstat instead of stat, because stat has a limitation that fstat doesn't have.

"No permissions are required on the file itself, but—in the case of stat(), fstatat(), and lstat() execute (search) permission is required on all of the directories in pathname that lead to the file.

The three functions stat, fstatat and lstat have execute permission restriction on the directories whose path is a prefix of the path name parameter of these functions. Those restriction only apply to functions with the path name parameter. In contrast to these three functions, fstat doesn't have a path parameter and therefore doesn't have these restrictions. Now you might think that using fstat avoids these restrictions and is therefore a better choice, but in reality, the open function which is being used to acquire the file descriptor fd has the exact same limitations with the permissions.

To illustrate the limitation I have the following example. Suppose I want to get the file information from a file with the path /a/b/c/d/text.txt. If I use stat then the directories./, /a, /a/b, /a/b/c, /a/b/c/d must all have the execute (search or traverse) permission in order for text.txt to be accessible. Otherwise, this file isn't accessible. That's the limitation. Now suppose to use fstat instead. In order to get the file information from /a/b/c/d/text.txt via fstat I need to open it first via the open function. That open function returns a file descriptor which I then supply it to fstat. Of course, that file descriptor has to be closed via close. While fstat doesn't require the /, /a, /a/b, /a/b/c, /a/b/c/d directories to have execute (search or traverse) permission, the open function does. Therefore, the execute permission is required for directories leading to text.txt in order to acquire the file information, no matter whether stat of fstat is being used.

Your changes in this PR can be simplified to the following code.

#define STAT_FUNC real_stat

...

#ifndef _WIN32

static int real_stat(const char *pathname, STAT_STRUCT *buf) {
	int fd = open(pathname, O_RDONLY);
	if(fd<0) {
		return -1;
	}
	int err1 = fstat(fd, buf);
	int err2 = close(fd);
	return !err1 && !err2 ? 0 : -1;
}

#endif

With the simplification, I created a new function called real_stat. real_stat internally uses fstat in order to avoid the execute permission limitation for directories leading to pathname. The problem with it is that open has this limitation. Therefore real_path has the exact same execute permission requirement for directories leading to pathname as stat. No matter how much this code gets turned and twisted, real_path gets still the same limitation. The execute permission requirement for directories leading to pathname cannot be avoided in the unix systems and actually makes perfect sense. These directories need execute (search or traverse) permission in order for the file in pathname to be reachable. In a certain way, real_stat is more limiting that stat. real_stat needs read permission for the file because of open(pathname, O_RDONLY); whereas stat doesn't need any permission on the file.

In summary. Looking from a logical perspective, using fstat instead of stat doesn't relax any restriction. In contrary, it add restrictions, because using fstat requires the open function which then requires some permission such as read. Also, the additional code adds maintenance burden.

@jaromil
Copy link
Author

jaromil commented Dec 7, 2024

Thanks for triaging, weirdly enough the fstat approach worked in my (long forgotten) use case, can't figure out why now, must have been a kernel or filesystem issue because what you say is right in theory. Ciao

@jaromil jaromil closed this Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants